Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow users to use their own implementation of FileMigrationLoader. R… #107

Merged
merged 1 commit into from
Dec 15, 2017

Conversation

harawata
Copy link
Member

@harawata harawata commented Dec 12, 2017

…elated to #93 #102 .

To use a custom FileMigrationLoader, you need to ...

  1. create an implementation of FileMigrationLoaderFactory [1].
  2. create a JAR including a file /META-INF/services/org.apache.ibatis.migration.FileMigrationLoaderFactory that contains fully-qualified name of the custom implementation (Migrations looks for a custom implementation via Java Service Provider Interface).
  3. drop the JAR file in $MIGRATIONS_HOME/lib directory.

[1] Note that the signature of FileMigrationLoaderFactory#create() is still under discussion and could be changed before final release.

Please see these examples (not tested thoroughly!).

@h3adache ,
I'm so sorry to keep bothering you, but could you please see if this change is acceptable or not when you have time?
Thanks in advance!

Copy link
Contributor

@chb0github chb0github left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we take this a level higher it could be really powerful. This would allow a fully codeable migration.

Properties variables = environment().getVariables();
MigrationLoader migrationLoader = null;
for (FileMigrationLoaderFactory factory : ServiceLoader.load(FileMigrationLoaderFactory.class)) {
if (migrationLoader != null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this idea. How about taking it one step higher to MigrationLoaderFactory so that it's no longer file based? Basically, you can abstract the Migration and get an iterator of them.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea I agree, we have a MigrationLoader so a MigrationLoader factory makes a lot of sense. The FileMigrationLoader is just an implementation.
It might be more useful to just give it the path(),environment() for flexibility.

Copy link
Member Author

@harawata harawata Dec 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you guys for the quick feedback!

How about taking it one step higher to MigrationLoaderFactory so that it's no longer file based?

I thought about that.
The return type of FileMigrationLoaderFactory#create() is MigrationLoader, so by design, it is possible to return a custom MigrationLoader that loads migrations from some other source (i.e. it does not have to be a subclass of FileMigrationLoader).

But this is BaseCommand which is the base class for the command line API and the current command line workflow cannot be separated from files completely (e.g. migrate new).
That is why I named it FileMigrationLoaderFactory.

It might be more useful to just give it the path(),environment() for flexibility.

It was my first implementation and I'm still not that sure which is better, so if you think that is better, I'll change. :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea it's tied to Files atm but we also have for example JavaMigrationLoader that is useful for runtime migrations.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw that. But

JavaMigrationLoader has a method getMigrations() which returns a List<Change> (counter conventional, btw).

When you look at Change you see

public class Change implements Comparable<Change>, Cloneable {

  private BigDecimal id;
  private String description;
  private String appliedTimestamp;
  private String filename;
}

Notice the part where it says fileName? It's still anchored to the file system.

Side question: Why is the id BigDecimal? Do you expect a version like 3.1415926?

Copy link
Member

@h3adache h3adache Dec 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what you mean @chb0github, both have getMigrations which returns a list of changes.

Even if we change the signature of create() method as @h3adache suggested, it still is coupled with file system and FileMigrationLoaderFactory describes the purpose of this class more accurately, IMO.

That is true. I was just thinking about it from a Factory standpoint. The factory author can create the loader any way they like so I just assumed they would have the proper classLoader/packageNames to create the JavaMigrationLoader

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@harawata if you have no more changes let's pull this in. Don't worry about the Factory idea for now. We can deal with it when we have to like you said

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@h3adache ,
Okay!
Regarding the arguments of create(), I re-checked SelectedPaths and Environment and couldn't think of a good use case for other properties.
Let's merge this as it is and see what users say.

I'll add documentation and tests as soon as possible.

Thank you both for insightful comments!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I use it to create a JarFileMigrationLoader. :)
ATM our FileMigrationLoader doesn't read jar bundled migrations properly.
I create a JarFileMigrationLoader to handle that for me but I read the properties from the bundled environment file in the jar as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@h3adache
Copy link
Member

This looks great @harawata!

@harawata harawata merged commit ad97fd5 into mybatis:master Dec 15, 2017
@harawata harawata self-assigned this Dec 15, 2017
@harawata harawata added this to the 3.3.2 milestone Dec 15, 2017
@h3adache
Copy link
Member

Thanks @harawata!

@chb0github
Copy link
Contributor

When will this be released?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants